Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @sarahcaseybot and @weiminyu)

core/src/main/java/google/registry/bsa/BsaEmailSender.java line 24 at r1 (raw file):

/** Sends BSA-related email notifications. */
class BsaEmailSender {

I wonder if it is worth it to have a sender class just as a simple wrapper around GmailClient. It seems you could just easily inject the GmailClient and the recipient address at sites where you need to send emails. Why bother with another layer of indirection where potential bugs could be introduced (and needs to be separately tested)?

core/src/main/java/google/registry/bsa/UploadBsaUnavailableDomainsAction.java line 129 at r1 (raw file):

      boolean isGcsSuccess = uploadToGcs(unavailableDomains, runTime);
      boolean isBsaSuccess = uploadToBsa(unavailableDomains, runTime);
      if (isBsaSuccess && isGcsSuccess) {

In general, do we want to send emails for successful conditions? Wouldn't that result in a lot of emails that people just ignore and in the end may cause actual failure emails to be missed as well?

core/src/test/java/google/registry/bsa/UploadBsaUnavailableDomainsActionTest.java line 108 at r1 (raw file):

    assertThat(blockList).doesNotContain("not-blocked.tld");

    // This test currently fails in the upload-to-bsa step.

I don't understand what this means. Should it fail in some other steps? What is expected?

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jianglai and @sarahcaseybot)

core/src/main/java/google/registry/bsa/BsaEmailSender.java line 24 at r1 (raw file):

A sender class can serve two purposes:

1. It manages the recipient addresses so that the caller doesn't have to worry about. Granted, we have only one recipient right now but it doesn't hurt.
2. It can decorate the message body with action-wide boiler-plates.

It's a bit easier to write tests too, without having to create EmailMessage in every test.

The GmailClient code path is covered in some tests.

core/src/main/java/google/registry/bsa/UploadBsaUnavailableDomainsAction.java line 129 at r1 (raw file):

I think yes. We want to know they have run, at least initially.

People can also filter out the successful notifications easily.

core/src/test/java/google/registry/bsa/UploadBsaUnavailableDomainsActionTest.java line 108 at r1 (raw file):

Ben needs to fix the test. See todo below.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sarahcaseybot)